Automated Test: feature-recovery-keys-implementation #328
Conversation
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
📝 WalkthroughWalkthroughThis pull request introduces centralized support for recovery authentication codes across the Keycloak authentication system. New utility methods and helper functions provide consistent credential creation and retrieval for recovery codes. The authentication flows, required actions, and login forms are updated to use these utilities. Test infrastructure extends backwards-compatibility user storage with recovery code support and adds verification scenarios. Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant App as Login Application
participant RecoveryAction as RecoveryAuthnCodesAction
participant Helper as CredentialHelper
participant Provider as CredentialProvider
participant Storage as User Storage
rect rgba(200, 150, 255, 0.5)
Note over User,Storage: Recovery Code Setup Flow
User->>App: Initiate recovery code setup
App->>RecoveryAction: Handle required action
RecoveryAction->>RecoveryAction: Generate recovery codes
RecoveryAction->>Helper: createRecoveryCodesCredential(...)
Helper->>Helper: Serialize codes to JSON
alt Code in User Storage
Helper->>Storage: Save credential to user storage
Storage-->>Helper: Confirmed
else Code via Provider
Helper->>Provider: Create credential via provider
Provider-->>Helper: Credential created
end
Helper-->>RecoveryAction: Credential stored
RecoveryAction-->>User: Display recovery codes
end
rect rgba(150, 200, 255, 0.5)
Note over User,Storage: Recovery Code Login Flow
User->>App: Authenticate with recovery code
App->>App: Retrieve user
App->>Storage: Get credential via RecoveryAuthnCodesUtils
Storage-->>App: Optional<CredentialModel>
alt Credential Present
App->>App: Deserialize & validate code
App-->>User: Authentication successful
else Credential Missing
App-->>User: Authentication failed
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java (1)
80-92:⚠️ Potential issue | 🟠 MajorFederated recovery credential removal will fail silently.
RecoveryAuthnCodesUtils.getCredential()checks federated credentials first viagetFederatedCredentialsStream(). However,removeStoredCredentialById()(lines 86-87) operates only on local storage and cannot remove federated credentials. If a federated credential exists andallCodesUsed()returns true, the removal attempt silently fails and the stale federated credential persists. The fallback at lines 90-92 (addingCONFIGURE_RECOVERY_AUTHN_CODESas a required action) mitigates this partially but does not clean up the orphaned credential.
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`:
- Around line 17-21: The code calls credentialModelOpt.get() and then
getNextRecoveryAuthnCode().get() without presence checks, which can throw
NoSuchElementException; update RecoveryAuthnCodeInputLoginBean to first check
credentialModelOpt.isPresent() (or use
credentialModelOpt.map(...).ifPresent/else) before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel and
getNextRecoveryAuthnCode(), and if absent set this.codeNumber to null (or an
appropriate default) so the form renders gracefully, mirroring the presence
check logic used in RecoveryAuthnCodesFormAuthenticator.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`:
- Around line 123-143: In configureBrowserFlowWithRecoveryAuthnCodes, fix the
typo in the alias passed to config.setAlias for the delayed authenticator:
change "delayed-suthenticator-config" to "delayed-authenticator-config" so the
alias matches the authenticator ID "delayed-authenticator" and config lookup;
locate the block that adds the "delayed-authenticator" execution and update the
string in config.setAlias accordingly.
- Around line 240-269: The testRecoveryKeysSetupAndLogin test calls
testAppHelper.logout() without asserting its boolean result; update the teardown
to assert the logout succeeded by replacing the bare call with
assertTrue(testAppHelper.logout(), "logout should succeed") (or equivalent
assertTrue usage already used elsewhere in the class) so the test fails if
logout returns false; ensure you reference testAppHelper.logout() and use the
existing assertTrue assertion style from other tests.
- Around line 258-262: After submitting the recovery code form the test never
completes the OAuth flow; add a call to testAppHelper.completeLogin()
immediately after enterRecoveryAuthnCodePage.clickSignInButton() and before
appPage.assertCurrent() so the authorization code is exchanged for tokens
(mirrors the pattern used in testOTPUpdateAndLogin where startLogin(...), submit
2FA, completeLogin(), then appPage.assertCurrent()).
🧹 Nitpick comments (3)
testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java (2)
230-252: Local variablemodelshadows the class fieldthis.model(ComponentModel).Line 233 declares
RecoveryAuthnCodesCredentialModel modelwhich shadows the inheritedComponentModel modelfield (Line 72). Consider renaming torecoveryModelorrecoveryCredentialModelto avoid confusion.Proposed rename
public Stream<CredentialModel> getCredentials(RealmModel realm, UserModel user) { var myUser = getMyUser(user); - RecoveryAuthnCodesCredentialModel model; List<CredentialModel> credentialModels = new ArrayList<>(); if (myUser.recoveryCodes != null) { try { - model = RecoveryAuthnCodesCredentialModel.createFromValues( + RecoveryAuthnCodesCredentialModel recoveryModel = RecoveryAuthnCodesCredentialModel.createFromValues( JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), myUser.recoveryCodes.getCreatedDate(), myUser.recoveryCodes.getUserLabel() ); - credentialModels.add(model); + credentialModels.add(recoveryModel); } catch (IOException e) { log.error("Could not deserialize credential of type: recovery-codes"); }
326-341: Raw typeList— useList<String>for type safety.Lines 332 and 334 use raw
Listwithout type parameters. UsingList<String>would provide compile-time type safety and avoid an unchecked cast warning when comparing withinput.getChallengeResponse()(aString) on Line 340.Proposed fix
- List generatedKeys; + List<String> generatedKeys; try { - generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); + `@SuppressWarnings`("unchecked") + List<String> keys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); + generatedKeys = keys; } catch (IOException e) {testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java (1)
476-483: IssetDriver(driver)necessary here?The
enterRecoveryAuthnCodePageis already injected via@Page(line 104), which should configure the driver automatically. IfsetDriveris needed as a workaround (e.g., for Arquillian page-object lifecycle quirks), a brief comment would help future readers understand why.
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | ||
|
|
||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | ||
|
|
||
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); |
There was a problem hiding this comment.
Unchecked Optional.get() will throw NoSuchElementException if no credential exists.
Line 19 calls credentialModelOpt.get() without an isPresent() guard. If neither federated nor stored credentials are found, this throws a NoSuchElementException, resulting in a 500 error during form rendering.
By contrast, RecoveryAuthnCodesFormAuthenticator (Line 82) correctly checks optUserCredentialFound.isPresent() before calling .get().
🐛 Proposed fix: handle empty Optional gracefully
- Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
-
- RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
-
- this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber();
+ Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
+
+ RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = credentialModelOpt
+ .map(RecoveryAuthnCodesCredentialModel::createFromCredentialModel)
+ .orElseThrow(() -> new IllegalStateException("Recovery authentication codes credential not found for user"));
+
+ this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode()
+ .orElseThrow(() -> new IllegalStateException("No remaining recovery authentication codes"))
+ .getNumber();🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`
around lines 17 - 21, The code calls credentialModelOpt.get() and then
getNextRecoveryAuthnCode().get() without presence checks, which can throw
NoSuchElementException; update RecoveryAuthnCodeInputLoginBean to first check
credentialModelOpt.isPresent() (or use
credentialModelOpt.map(...).ifPresent/else) before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel and
getNextRecoveryAuthnCode(), and if absent set this.codeNumber to null (or an
appropriate default) so the form renders gracefully, mirroring the presence
check logic used in RecoveryAuthnCodesFormAuthenticator.
| void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { | ||
| final String newFlowAlias = BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES; | ||
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); | ||
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) | ||
| .selectFlow(newFlowAlias) | ||
| .inForms(forms -> forms | ||
| .clear() | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, UsernamePasswordFormFactory.PROVIDER_ID) | ||
| .addSubFlowExecution(AuthenticationExecutionModel.Requirement.REQUIRED, reqSubFlow -> reqSubFlow | ||
| .addSubFlowExecution("Recovery-Authn-Codes subflow", AuthenticationFlow.BASIC_FLOW, AuthenticationExecutionModel.Requirement.ALTERNATIVE, altSubFlow -> altSubFlow | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, RecoveryAuthnCodesFormAuthenticatorFactory.PROVIDER_ID) | ||
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { | ||
| config.setAlias("delayed-suthenticator-config"); | ||
| config.setConfig(Map.of("delay", Long.toString(delay))); | ||
| }) | ||
| ) | ||
| ) | ||
| ) | ||
| .defineAsBrowserFlow() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Typo in config alias: "delayed-suthenticator-config" → "delayed-authenticator-config".
Line 135 has a typo — suthenticator instead of authenticator.
✏️ Proposed fix
- config.setAlias("delayed-suthenticator-config");
+ config.setAlias("delayed-authenticator-config");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { | |
| final String newFlowAlias = BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES; | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) | |
| .selectFlow(newFlowAlias) | |
| .inForms(forms -> forms | |
| .clear() | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, UsernamePasswordFormFactory.PROVIDER_ID) | |
| .addSubFlowExecution(AuthenticationExecutionModel.Requirement.REQUIRED, reqSubFlow -> reqSubFlow | |
| .addSubFlowExecution("Recovery-Authn-Codes subflow", AuthenticationFlow.BASIC_FLOW, AuthenticationExecutionModel.Requirement.ALTERNATIVE, altSubFlow -> altSubFlow | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, RecoveryAuthnCodesFormAuthenticatorFactory.PROVIDER_ID) | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { | |
| config.setAlias("delayed-suthenticator-config"); | |
| config.setConfig(Map.of("delay", Long.toString(delay))); | |
| }) | |
| ) | |
| ) | |
| ) | |
| .defineAsBrowserFlow() | |
| ); | |
| } | |
| void configureBrowserFlowWithRecoveryAuthnCodes(KeycloakTestingClient testingClient, long delay) { | |
| final String newFlowAlias = BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES; | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); | |
| testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) | |
| .selectFlow(newFlowAlias) | |
| .inForms(forms -> forms | |
| .clear() | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, UsernamePasswordFormFactory.PROVIDER_ID) | |
| .addSubFlowExecution(AuthenticationExecutionModel.Requirement.REQUIRED, reqSubFlow -> reqSubFlow | |
| .addSubFlowExecution("Recovery-Authn-Codes subflow", AuthenticationFlow.BASIC_FLOW, AuthenticationExecutionModel.Requirement.ALTERNATIVE, altSubFlow -> altSubFlow | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, RecoveryAuthnCodesFormAuthenticatorFactory.PROVIDER_ID) | |
| .addAuthenticatorExecution(AuthenticationExecutionModel.Requirement.REQUIRED, "delayed-authenticator", config -> { | |
| config.setAlias("delayed-authenticator-config"); | |
| config.setConfig(Map.of("delay", Long.toString(delay))); | |
| }) | |
| ) | |
| ) | |
| ) | |
| .defineAsBrowserFlow() | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`
around lines 123 - 143, In configureBrowserFlowWithRecoveryAuthnCodes, fix the
typo in the alias passed to config.setAlias for the delayed authenticator:
change "delayed-suthenticator-config" to "delayed-authenticator-config" so the
alias matches the authenticator ID "delayed-authenticator" and config lookup;
locate the block that adds the "delayed-authenticator" execution and update the
string in config.setAlias accordingly.
| @Test | ||
| public void testRecoveryKeysSetupAndLogin() throws URISyntaxException, IOException { | ||
| try { | ||
| configureBrowserFlowWithRecoveryAuthnCodes(testingClient, 0); | ||
|
|
||
| String userId = addUserAndResetPassword("otp1", "pass"); | ||
| getCleanup().addUserId(userId); | ||
|
|
||
| // Setup RecoveryKeys | ||
| List<String> recoveryKeys = setupRecoveryKeysForUserWithRequiredAction(userId, true); | ||
|
|
||
| // Assert user has RecoveryKeys in the userStorage | ||
| assertUserDontHaveDBCredentials(); | ||
| assertUserHasRecoveryKeysCredentialInUserStorage(true); | ||
|
|
||
| TestAppHelper testAppHelper = new TestAppHelper(oauth, loginPage, appPage); | ||
|
|
||
| // Authenticate as the user | ||
| testAppHelper.startLogin("otp1", "pass"); | ||
| enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys); | ||
| enterRecoveryAuthnCodePage.clickSignInButton(); | ||
|
|
||
| appPage.assertCurrent(); | ||
|
|
||
| testAppHelper.logout(); | ||
| } finally { | ||
| // Revert copy of browser flow to original to keep clean slate after this test | ||
| BrowserFlowTest.revertFlows(testRealm(), BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing assertion on logout() return value.
At line 264, testAppHelper.logout() is called without wrapping it in assertTrue(...), unlike other tests in this class (e.g., lines 237, 405). A failed logout would silently pass.
✏️ Proposed fix
- testAppHelper.logout();
+ assertTrue(testAppHelper.logout());🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`
around lines 240 - 269, The testRecoveryKeysSetupAndLogin test calls
testAppHelper.logout() without asserting its boolean result; update the teardown
to assert the logout succeeded by replacing the bare call with
assertTrue(testAppHelper.logout(), "logout should succeed") (or equivalent
assertTrue usage already used elsewhere in the class) so the test fails if
logout returns false; ensure you reference testAppHelper.logout() and use the
existing assertTrue assertion style from other tests.
| testAppHelper.startLogin("otp1", "pass"); | ||
| enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys); | ||
| enterRecoveryAuthnCodePage.clickSignInButton(); | ||
|
|
||
| appPage.assertCurrent(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the EnterRecoveryAuthnCodePage class
fd "EnterRecoveryAuthnCodePage.java" --exec cat {} 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2210
🏁 Script executed:
#!/bin/bash
# Also examine the test file around the lines in question
fd "BackwardsCompatibilityUserStorageTest.java" --exec sed -n '210,270p' {} 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2302
🏁 Script executed:
#!/bin/bash
# Search for clickSignInButton usage patterns in test files
rg "clickSignInButton" -A 3 -B 3 --max-count 20 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 9068
🏁 Script executed:
#!/bin/bash
# Find and examine TestAppHelper.completeLogin() method
fd "TestAppHelper.java" --exec grep -A 10 "completeLogin" {} 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1337
🏁 Script executed:
#!/bin/bash
# Search for assertCurrent implementations in AppPage and similar classes
rg "assertCurrent\(\)" -A 5 "AppPage" --max-count 10 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Look for more context about how tests complete 2FA flows
rg "clickSignInButton\(\)" -A 5 "appPage" 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Verify the pattern - check if there are any other tests that skip completeLogin after 2FA
rg "enterRecoveryCodes|clickSignInButton" -B 2 -A 5 "BackwardsCompatibilityUserStorageTest" 2>/dev/nullRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Check if there are recovery code tests in other files that DO call completeLogin
rg "enterRecoveryCodes.*recovery" -A 10 2>/dev/null | head -40Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2100
Add testAppHelper.completeLogin() call before the assertion.
The completeLogin() method is required to exchange the OAuth authorization code for tokens. Without it, the test skips the entire token exchange phase and may pass without properly validating the OAuth flow completion. This is inconsistent with the testOTPUpdateAndLogin method in the same class, which correctly calls completeLogin() after submitting the second authentication factor.
Current code:
testAppHelper.startLogin("otp1", "pass");
enterRecoveryCodes(enterRecoveryAuthnCodePage, driver, 0, recoveryKeys);
enterRecoveryAuthnCodePage.clickSignInButton();
appPage.assertCurrent();
Add testAppHelper.completeLogin(); after clickSignInButton() and before appPage.assertCurrent().
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/BackwardsCompatibilityUserStorageTest.java`
around lines 258 - 262, After submitting the recovery code form the test never
completes the OAuth flow; add a call to testAppHelper.completeLogin()
immediately after enterRecoveryAuthnCodePage.clickSignInButton() and before
appPage.assertCurrent() so the authorization code is exchanged for tokens
(mirrors the pattern used in testOTPUpdateAndLogin where startLogin(...), submit
2FA, completeLogin(), then appPage.assertCurrent()).
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
Improvements
Tests